Closes #6432: Bail out from beacon conditions #6555

Miraeld · 2024-04-10T12:11:33Z

Description

This PR modifies the beacon script to add a check for existing image data related to the URL in the database. If such data exists or the status is failed, the script process should be stopped. Additionally, a screen-size check should be added to the script to bail out under certain conditions.

Documentation

User documentation

This change does not directly impact users as it is related to the test setup and the beacon script.

Technical documentation

The add_lcp_data and check_lcp_data methods in the Controller.php file are used to add and check LCP data respectively. The main function in the lcp-beacon.js file has been modified to make an AJAX call to check if there are any records for the current URL and to check the screen size. If the screen size is not within the acceptable range, the function bails out.

Type of change

Enhancement (non-breaking change which improves an existing functionality).

New dependencies

N/A

Risks

N/A

Checklists

Feature validation

I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
I triggered all changed lines of code at least once without new errors/warnings/notices.
I implemented built-in tests to cover the new/changed code.

Documentation

I prepared the user documentation for the feature/enhancement and shared it in the PR or the GitHub issue.
The user documentation covers new/changed entry points (endpoints, WP hooks, configuration files, ...).
I prepared the technical documentation if needed, and shared it in the PR or the GitHub issue.

Code style

I wrote self-explanatory code about what it does.
I wrote comments to explain why it does it.
I named variables and functions explicitly.
I protected entry points against unexpected inputs.
I did not introduce unnecessary complexity.
I listed the introduced external dependencies explicitly on the PR.
I validated the repo-specific guidelines from CONTRIBUTING.md.

Acceptance Criteria Reminder

Bail-out if data is already available:

Make sure there is a log in the beacon script in case it bails out because data is already available.
Clear cache and ATF/LCP data. Make sure cache feature and LCP/ATF feature are enabled.
Visit a page from desktop.
Check in the DB that the LCP/ATF data is available for this page.
Visit the page once again from desktop. Check in the dev tool console that the script about bailing out for data already there is printed out.
Visit the page from mobile. Check that there is no log about bailing out for data already there.
Check in the DB that the mobile LCP/ATF data has been added.

Bail-out if not acceptable screen-size: (Use BrowserStack or Rocket-E2E)

Check the screen-size filters:

Repeat the above test about screen-size after applying different upper and lower limits with a filter:
- Just set the upper limit: repeat the test (adapting the emulated screen-size).
- Just set the lower limit: repeat the test (adapting the emulated screen-size).
Validate that the filter is protected against unexpected inputs (null, empty, text, etc...)

remyperona · 2024-04-11T20:05:32Z

Is this ready for review?

Miraeld · 2024-04-12T08:11:27Z

@Tabrisrp, yes it is ready.
We keep it like this at the moment. The documentation related to these changes can be found here.

assets/js/lcp-beacon.js

inc/Engine/Media/AboveTheFold/AJAX/Controller.php

remyperona · 2024-04-16T21:23:51Z

You have some conflicts to fix in the JS

…nt/6432-bail-out-from-beacon-conditions

codacy-production · 2024-04-17T01:30:28Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -0.01% (target: -0.10%)	❌ 30.30% (target: 50.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`2486a81`)	36946	14155	38.31%
Head commit (`d783638`)	36964 (+18)	14160 (+5)	38.31% (-0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6555)	33	10	30.30%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy will stop sending the deprecated coverage status from June 5th, 2024. Learn more}

…ditions

…nt/6432-bail-out-from-beacon-conditions

MathieuLamiot · 2024-04-24T16:22:10Z

@Miraeld There are conflicts to be resolved. Then it should be merged ASAP to avoid further conflicts with upcoming changes on the beacon.

…nt/6432-bail-out-from-beacon-conditions

Miraeld · 2024-04-25T08:45:43Z

@Mai-Saad, there is no test plan available for this yet,
However, I've tested this PR.
The way I've tested => Open a page with different dimensions while checking the devconsole for results.

Please find screenshots of the result as follow:

Desktop

Case 1

Opening a page in desktop mode while not meeting the size resolution requirements, we bail out.

Case 2

Opening a page in desktop while meeting the size resolution, we process and get results.

Case 3

Opening a page in desktop while we already have saved data within the database.

Mobile

Case 1

Opening a page in mobile mode while not meeting the size resolution requirements, we bail out.

Case 2

Opening a page in mobile while meeting the size resolution, we process and get results.

Case 3

Opening a page in mobile while we already have saved data within the database.

Please review this results for now and if everything's okay, we can move it to QA Done.
Thank you

MathieuLamiot · 2024-04-25T11:03:08Z

@Miraeld Maybe we need to test the new filters and also that they are guarded against bad values? (null, [], string, etc.). If wrong value, then the default value should be applied.

Miraeld · 2024-04-25T14:08:17Z

True that, Here is the following tests:

Changing values of the filter

Settings new threshold:

// Change the width threshold for the LCP beacon.
add_filter( 'rocket_lcp_width_threshold', function( $width_threshold, $is_mobile, $url ) {
	return $is_mobile ? 500 : 2500;
}, 10, 3 );

// Change the height threshold for the LCP beacon.
add_filter( 'rocket_lcp_height_threshold', function( $height_threshold, $is_mobile, $url ) {
	return $is_mobile ? 900 : 1200;
}, 10, 3 );

Thresholds are working perfectly.

Passing `null` & `[]` to filters

// Change the width threshold for the LCP beacon.
add_filter( 'rocket_lcp_width_threshold', function( $width_threshold, $is_mobile, $url ) {
	return [];
}, 10, 3 );

// Change the height threshold for the LCP beacon.
add_filter( 'rocket_lcp_height_threshold', function( $height_threshold, $is_mobile, $url ) {
	return null;
}, 10, 3 );

Result:

As values from the filter were not valid, we set back the default value.

Also please see #6593 for new threshold values modification

MathieuLamiot · 2024-04-25T16:17:24Z

Ok on my side 👍

Miraeld changed the base branch from develop to feature/lcp-above-the-fold-optimization April 10, 2024 12:11

Miraeld added this to the 3.16 milestone Apr 10, 2024

Miraeld force-pushed the enhancement/6432-bail-out-from-beacon-conditions branch from dcd698f to 4e3757f Compare April 10, 2024 13:25

Miraeld changed the title ~~Enhancement/6432 bail out from beacon conditions~~ Closes #6432: Bail out from beacon conditions Apr 10, 2024

Miraeld force-pushed the enhancement/6432-bail-out-from-beacon-conditions branch from 4e3757f to 592a01e Compare April 10, 2024 13:54

Closes #6432: Adds conditions to bail out from beacon

1308890

Miraeld force-pushed the enhancement/6432-bail-out-from-beacon-conditions branch from 592a01e to 1308890 Compare April 10, 2024 13:58

Fix integrations tests

1b02783

Miraeld requested a review from a team April 12, 2024 08:11

Miraeld self-assigned this Apr 12, 2024

remyperona requested changes Apr 12, 2024

View reviewed changes

assets/js/lcp-beacon.js Outdated Show resolved Hide resolved

assets/js/lcp-beacon.js Outdated Show resolved Hide resolved

inc/Engine/Media/AboveTheFold/AJAX/Controller.php Outdated Show resolved Hide resolved

Comply feedback

58f8343

Miraeld requested a review from remyperona April 15, 2024 14:21

remyperona approved these changes Apr 16, 2024

View reviewed changes

Miraeld added 2 commits April 17, 2024 03:25

Merge branch 'feature/lcp-above-the-fold-optimization' into enhanceme…

73f603e

…nt/6432-bail-out-from-beacon-conditions

Update lcp-beacon

538fbb1

Khadreal approved these changes Apr 17, 2024

View reviewed changes

Miraeld added 3 commits April 19, 2024 09:54

Merge branch 'develop' into enhancement/6432-bail-out-from-beacon-con…

ac6cefa

…ditions

Change conditions to bail out

ed582a8

Merge branch 'feature/lcp-above-the-fold-optimization' into enhanceme…

3e43699

…nt/6432-bail-out-from-beacon-conditions

Miraeld requested review from remyperona and Khadreal April 19, 2024 07:57

MathieuLamiot approved these changes Apr 19, 2024

View reviewed changes

MathieuLamiot mentioned this pull request Apr 24, 2024

3.16 - Remove console.log of the beacon script when the debug mode is not enabled #6574

Closed

Merge branch 'feature/lcp-above-the-fold-optimization' into enhanceme…

549d524

…nt/6432-bail-out-from-beacon-conditions

Miraeld added 3 commits April 25, 2024 04:19

Update lcp-beacon.min.js

b174327

Fix linter

bad9d46

Fix linter

d783638

Miraeld merged commit 3f46bf6 into feature/lcp-above-the-fold-optimization Apr 25, 2024
11 of 13 checks passed

Miraeld deleted the enhancement/6432-bail-out-from-beacon-conditions branch April 25, 2024 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #6432: Bail out from beacon conditions #6555

Closes #6432: Bail out from beacon conditions #6555

Miraeld commented Apr 10, 2024 •

edited

Loading

remyperona commented Apr 11, 2024

Miraeld commented Apr 12, 2024

remyperona commented Apr 16, 2024

codacy-production bot commented Apr 17, 2024 •

edited

Loading

MathieuLamiot commented Apr 24, 2024

Miraeld commented Apr 25, 2024

MathieuLamiot commented Apr 25, 2024

Miraeld commented Apr 25, 2024

MathieuLamiot commented Apr 25, 2024

Closes #6432: Bail out from beacon conditions #6555

Closes #6432: Bail out from beacon conditions #6555

Conversation

Miraeld commented Apr 10, 2024 • edited Loading

Description

Documentation

User documentation

Technical documentation

Type of change

New dependencies

Risks

Checklists

Feature validation

Documentation

Code style

Acceptance Criteria Reminder

remyperona commented Apr 11, 2024

Miraeld commented Apr 12, 2024

remyperona commented Apr 16, 2024

codacy-production bot commented Apr 17, 2024 • edited Loading

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

MathieuLamiot commented Apr 24, 2024

Miraeld commented Apr 25, 2024

Desktop

Case 1

Case 2

Case 3

Mobile

Case 1

Case 2

Case 3

MathieuLamiot commented Apr 25, 2024

Miraeld commented Apr 25, 2024

Changing values of the filter

Settings new threshold:

Passing null & [] to filters

MathieuLamiot commented Apr 25, 2024

Miraeld commented Apr 10, 2024 •

edited

Loading

codacy-production bot commented Apr 17, 2024 •

edited

Loading

Passing `null` & `[]` to filters